-
Notifications
You must be signed in to change notification settings - Fork 17
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[FC-0072] iOS Mobile Plugin Architecture #528
[FC-0072] iOS Mobile Plugin Architecture #528
Conversation
Thanks for the pull request, @IvanStepanok! What's next?Please work through the following steps to get your changes ready for engineering review: 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Let us know that your PR is ready for review:Who will review my changes?This repository is currently maintained by Where can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:
When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
2aa83ba
to
16fd879
Compare
@@ -9,6 +9,7 @@ import SwiftyMocky | |||
import XCTest | |||
@testable import Core | |||
@testable import Profile | |||
import OEXFoundation | |||
import Alamofire |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you moved Alamofire to OEXFoundation i think then we should remove import that framework from main base code. Need to implement accessors for alamofire requests. For my side it would be logic that you just import OEXFoundation and able to use Alamofire's methods. Accessors could be named same as Alamofire's methods.
Same for other moved frameworks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the suggestion! We discussed the idea of creating wrappers for all libraries, including Alamofire, and concluded that at this stage, it would add complexity and slow down development.
The main reasons we decided to stick with direct imports for now are:
Need to cover all functionalities — creating wrappers for all required methods would take significant time, and in the future, it could slow down development as each new need would require adding a wrapper first.
Potential loss of flexibility — using libraries directly provides immediate access to all their functionalities. With wrappers, access to new or specialized functions may be limited, making development more challenging.
Increased maintenance costs — every new library version might require updating wrappers and verifying compatibility, leading to additional maintenance overhead.
We’ll definitely revisit this idea if a more isolated architecture is needed, but for now, we’ve decided to keep direct imports to ensure greater flexibility and development speed.
And one more - we need to think about procedure or maybe PR format style when you want to make changes to OEXFoundation and upstream code at same time. Maybe we just should add link each to other, maybe something else. Just to reviewer could see for what feature author made changes in OEXFoundation. |
Makes sense 👍 |
@@ -13,7 +13,6 @@ public extension Notification.Name { | |||
static let onCourseEnrolled = Notification.Name("onCourseEnrolled") | |||
static let onblockCompletionRequested = Notification.Name("onblockCompletionRequested") | |||
static let onTokenRefreshFailed = Notification.Name("onTokenRefreshFailed") | |||
static let onActualVersionReceived = Notification.Name("onActualVersionReceived") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
only this one is moved to OEXFoundation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenEdX/Managers/PluginManager.swift
Outdated
import Foundation | ||
import Core | ||
import OEXFoundation | ||
import OEXFirebaseAnalytics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this import here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
import UIKit | ||
import UserNotifications | ||
import OEXFirebaseAnalytics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this import here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
@@ -7,6 +7,8 @@ | |||
|
|||
import Foundation | |||
import Core | |||
import OEXFoundation | |||
import OEXFirebaseAnalytics |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this OEXFirebaseAnalytics import here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OEXFoundation needed for API class
OEXFirebaseAnalytics removed
config_folder = config_settings.get(self.CONFIG_MAPPINGS, {}).get(config['env_config']) | ||
if config_folder: | ||
# replace fullstory flag | ||
project_file_string = self.replace_fullstory_flag(project_file_string, config_directory, name, config_folder, errors_texts) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@IvanStepanok Maybe it would be better to comment out that line, rather than removing the whole ‘for’ loop?
Without this part, the whole set_flags_from_mobile_config(self):
function is meaningless. We could leave this as an example how to work with other flags in project file. WDYT? Thank you
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
makes sense, thanks!
@rnr Thanks for review, ready for the next part! |
Plugin System Integration for Open edX iOS Mobile Application
This PR introduces a plugin architecture to the Open edX mobile app for iOS platforms. By modularizing functionalities such as analytics and external dependencies into plugins, this enhancement aims to improve the app's extensibility and scalability. It allows for company-specific customizations without the need to modify the core codebase.
Key Changes:
Plugin Interface and Manager:
OEXFoundation
, enabling developers to create and integrate custom plugins seamlessly.PluginManager
to handle the initialization and management of plugins within the application.Core Application Update:
FirebaseAnalytics
andFullStory
from the core project.Firebase Analytics Plugin:
FirebaseAnalytics
using the newOEXFirebaseAnalytics
plugin, conforming to the analytics protocols defined inOEXFoundation
.Library Migration to OEXFoundation:
OEXFoundation
package for centralized management:Alamofire
(Networking)Kingfisher
(Image Loading)SwiftUI-Introspect
(UI Enhancements)Swinject
(Dependency Injection)SwiftLintPlugins
(Linting)Objectives:
Extensibility:
Customization:
Separation of Concerns:
Scalability:
Additional Notes:
Plugin Development:
OEXFoundation
.Version Consistency:
OEXFoundation
used in the core app and plugins are consistent to maintain compatibility.This update makes the Open edX iOS mobile app more adaptable to various business needs, fostering a sustainable and flexible framework for ongoing development. By adopting a plugin-based architecture, the app is better positioned for future enhancements and contributions from the developer community.